Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update/modal component #19997

Closed
wants to merge 6 commits into from
Closed

Update/modal component #19997

wants to merge 6 commits into from

Conversation

fernandovbs
Copy link
Contributor

Description

This pull request is related to #7698.
Updated focusOnMount prop so it can be defined as string or boolean. If "firstElement" is set as value, the first tabbable element will be focused. If any other truthy value is set, then the container will be focused.

How has this been tested?

I will create another pull request with changes to the component stories. It covers possible values to the prop.

Checklist:

  • [:x: ] My code is tested.
  • [ ❌ ] My code follows the WordPress code style.
  • [ ❌ ] My code follows the accessibility standards.
  • [ ❌ ] My code has proper inline documentation.

@fernandovbs fernandovbs mentioned this pull request Feb 2, 2020
Closed
@ItsJonQ ItsJonQ added the [Package] Components /packages/components label Feb 10, 2020
@ItsJonQ
Copy link

ItsJonQ commented Feb 10, 2020

@fernandovbs Haiii! I left a comment re: the updated focusOnMount logic.

I saw your other PR as well re: Storybook updates for Modal. I don't think it needs to be a separate PR.
Are you open to adding those changes from #19999 to this PR?

That way, it can all ship as a single merge :)

Final note! We'll need to update the Modal README.md as well to reflect the new API change.

Thanks again!!

@draganescu draganescu mentioned this pull request Apr 20, 2020
Closed
@aduth
Copy link
Member

aduth commented Apr 20, 2020

This pull request is related to #7698.

Can you clarify if you'd intended for this to close (satisfy the requirements of) #7698?

@fernandovbs
Copy link
Contributor Author

Hello @aduth. Yes, the idea here is to solve the issue.
I've updated the component to accept a string or boolean for the focusOnMount prop.
The default value is 'firstElement', but you can also set the value to 'container', true or false, where 'container' and true have the same behavior.
If you have any suggestions about how to make it better, please let me know.

@paaljoachim
Copy link
Contributor

paaljoachim commented Feb 20, 2021 •
edited
Loading

Here is another modal component by @youknowriad
#28574

@draganescu
Copy link
Contributor

Howdy @fernandovbs lovely PR! What is a way to test this? Also we seem to have a conflict. Other than that the change is super welcome.

Base automatically changed from master to trunk March 1, 2021 15:43
@fernandovbs fernandovbs force-pushed the update/modal-component branch from 7a3f74d to 5db2fc4 Compare May 15, 2021 22:08
@fernandovbs
Copy link
Contributor Author

Hi @draganescu I think that the changes introduced in #27699 have made this PR unnecessary. I'm closing it. Thanks for taking a look.

@fernandovbs fernandovbs deleted the update/modal-component branch May 15, 2021 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ItsJonQ ItsJonQ ItsJonQ left review comments

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees
No one assigned
Labels
[Package] Components /packages/components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fernandovbs @ItsJonQ @aduth @paaljoachim @draganescu